Snow Leopards - Geiselle H. and Justina L. by justinakliu · Pull Request #21 · Ada-C18/solar-system-api

justinakliu · 2022-10-25T18:07:38Z

No description provided.

anselrognlie

Waves 1 and 2 look great! Just a couple of suggestions for going forward.

anselrognlie · 2022-10-28T16:36:29Z

app/__init__.py

    app = Flask(__name__)

+    from .routes import bp
+    app.register_blueprint(bp)


anselrognlie · 2022-10-28T16:37:10Z

app/routes.py

+        self.description = description
+        self.moons = moons
+
+    def to_planet_dict(self):


👀 The live code showed using a name like to_cat_dict, so I can see why you used to_planet_dict here. But it will be more useful going forward to all this simply to_dict going forward. If we have multiple models in our application, we like to have a method with a standardized name across all of them for converting them into a dictionary.

Consider the __str__ method that we overrode in Swap Meet. Each item subclass didn't have a differently name version of __str__ (e.g., __electronics_str__). They had a method with the same name so they could all be treated similarly.

So I'd suggest renaming this to to_dict and leave the type information to be implied by the instance we're calling this on. Most likely, the planet we're working with would be stored in a variable called planet. So with the current name, we end up writing planet.to_planet_dict(), which feels a little redundant. If we rename this to to_dict, it would become simply planet.to_dict().

anselrognlie · 2022-10-28T16:37:39Z

app/routes.py

+        self.moons = moons
+
+    def to_planet_dict(self):
+        return dict(


👍 I like the dict() method for creating new dictionaries as well, since I can leave the quotes off the keys!

anselrognlie · 2022-10-28T16:38:03Z

app/routes.py

+    Planet(3, "Earth", "where we live", 1)
+]
+
+def validate_planet(planet_id):


anselrognlie · 2022-10-28T16:38:11Z

app/routes.py

+
+    abort(make_response({"message":f"planet {planet_id} not found"}, 404))
+
+bp = Blueprint("planets", __name__, url_prefix = "/planets")


anselrognlie · 2022-10-28T16:38:29Z

app/routes.py

+    results_list = []
+
+    for planet in planets:
+        results_list.append(planet.to_planet_dict())


👀 List comprehension opportunity here

anselrognlie · 2022-10-28T16:39:06Z

app/routes.py

+    return jsonify(results_list)
+
+@bp.route("/<planet_id>", methods=["GET"])
+def handle_one_planet(planet_id):


The Learn modules show naming the id matching part of the route with a model prefix. This can be helpful to distinguish among multiple ids in the route if we're working with nested routes.

When there's only a single id parameter in our route, we typically call it simply id as was done in the live code.

anselrognlie

👍 Keep it up for Task List API!

anselrognlie · 2022-11-10T02:10:28Z

app/__init__.py

+
+    from .routes import planet
+    from .routes import fictional_caretaker
+    app.register_blueprint(planet.bp)


anselrognlie · 2022-11-10T02:11:41Z

app/models/fictional_caretaker.py

@@ -0,0 +1,16 @@
+from app import db


Why "fictional" caretaker? You mean your planets don't have real caretakers? 🙂

Remember that we prefer to have a file containing a class definition to match the name of the class (allowing for differences in capitalization).

anselrognlie · 2022-11-10T02:14:59Z

app/routes/fictional_caretaker.py

+    db.session.add(new_caretaker)
+    db.session.commit()
+
+    return make_response(f"Caretaker {new_caretaker.name} successfully created", 201)


👀 Make sure to jsonify even plain string responses, otherwise flask will return an HTML response rather than JSON.

anselrognlie · 2022-11-10T02:16:18Z

app/routes/fictional_caretaker.py

+
+
+@bp.route("/<caretaker_id>/planets", methods=["GET"])
+def read_one_planet(caretaker_id):


maybe call this get_caretaker_planets?

anselrognlie · 2022-11-10T02:16:44Z

app/routes/helper_functions.py

@@ -0,0 +1,14 @@
+from flask import abort, make_response
+
+def validate_model(cls, model_id):


👍 Nice job on the reafactor, and moving it out of the original routes file.

anselrognlie · 2022-11-10T02:17:24Z

app/routes/planet.py

+
+    planet_query = Planet.query
+
+    if name_query:


👍 Nice multi-value filtering!

anselrognlie · 2022-11-10T02:18:20Z

app/routes/planet.py

+    planet.name = request_body["name"]
+    planet.description = request_body["description"]
+    planet.moons = request_body["moons"]


In the same way we made from_dict to centralize knowledge about the needed keys in the model itself, we could add a method like update_from_dict to apply updated values to an instance from a dictionary.

anselrognlie · 2022-11-10T02:18:53Z

migrations/versions/b3e2f14f62df_added_fictional_caretaker.py

@@ -0,0 +1,36 @@
+"""added fictional caretaker


Nice to see that you went through a few migrations. Keep in mind that once you've gotten your "final" table layour squared away that it can be helpful to clear out all th old migrations, recreate the empty database, and get 1 nice, clean migration. Not something we'd want to do after having real data in a production database, but it's nice to start without migration clutter.

anselrognlie · 2022-11-10T02:19:28Z

tests/conftest.py

+    return app.test_client()
+
+@pytest.fixture
+def one_saved_planet(app):


We would also want fixtures for the caretakers if there had been more time.

anselrognlie · 2022-11-10T02:20:04Z

tests/test_routes.py

+    response_body = response.get_data(as_text=True)
+    #Assert
+    assert response.status_code == 201
+    assert response_body == "Planet Mars successfully created"


It would be a good idea to follow this up with a get call to confirm that the planet actually got stored in the database.

geiselleholt and others added 5 commits October 21, 2022 12:08

Wave 1

49d78f0

corrected spelling of jsonify

a0e06c7

added additional attribute moons

8fe6e8d

Wave 2

07beac8

Added helper function for return dict

a7e88ce

anselrognlie reviewed Oct 28, 2022

View reviewed changes

justinakliu and others added 10 commits October 28, 2022 14:25

Wave 3

6340b31

Wave 4

639d4fe

created from_dict method for class Planets

0b62470

refactored validate planet to be validate model

0967274

Added query params to read_one_planet

fd2fbcf

Made helper function file, moved validate_model there

f469fb0

Wave 6

f200d01

moved test folder outside app folder

f714afa

Deleted test folder in app folder

4c671a1

caretaker one to many relationship, routes folder

acccfd3

anselrognlie reviewed Nov 10, 2022

View reviewed changes


		abort(make_response({"message":f"planet {planet_id} not found"}, 404))

		bp = Blueprint("planets", __name__, url_prefix = "/planets")



		@bp.route("/<caretaker_id>/planets", methods=["GET"])
		def read_one_planet(caretaker_id):

		@@ -0,0 +1,14 @@
		from flask import abort, make_response

		def validate_model(cls, model_id):

Conversation

justinakliu commented Oct 25, 2022

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants